Skip to content

fix(ingestion): own generic Rust inherent-impl methods through the mod-qualified Impl node (#1992)#2003

Merged
magyargergo merged 7 commits into
mainfrom
fix/1992-rust-generic-impl-ownership
Jun 4, 2026
Merged

fix(ingestion): own generic Rust inherent-impl methods through the mod-qualified Impl node (#1992)#2003
magyargergo merged 7 commits into
mainfrom
fix/1992-rust-generic-impl-ownership

Conversation

@magyargergo

Copy link
Copy Markdown
Collaborator

Stacked PR 1/5 — base main (bottom of the #1981 follow-up stack). Merge this first; the next PR auto-retargets to main.

Summary

Follow-up to #1981 / #1982. bc4a560d mod-qualified the unscoped bare impl Inner target; a generic inherent-impl target (impl<T> Inner<T>) was not.

The Impl node was already correctly mod-qualified for generic impls (the @name capture drills into the inner type_identifier, tree-sitter-queries.ts:1226). The defect was in the owner walk (findEnclosingClassInfo): its inherent-impl arm matched only type_identifier / scoped_type_identifier, so a generic_type target fell through, the walk returned null, and the method got File → DEFINES with no HAS_METHOD edge — orphaned, and invisible to findDanglingEdges.

Note: this corrects the issue's stated mechanism. The failure is method orphaning, not node-collapse/cross-wire, and the fix is single-site (the owner walk) — the node-materialization gates already fire correctly and are left untouched.

Fix

Single site in findEnclosingClassInfo: match a generic_type target, drill into its base (childForFieldName('type')), and qualify a bare type_identifier base via the same qualifyRustImplTargetByModScope the node side uses → owner id == node id byte-for-byte (a.Inner / b.Inner). A scoped-generic base (impl<T> a::Inner<T>) materializes no @definition.impl node, so it falls through with no owner (orphaned, deferred) rather than minting a phantom owner. Node-typed and Rust-impl-gated — no other language reaches the impl_item arm; no shared-walker change for other languages.

Tests

New rust-nested-tail-collision-generic fixture + tests:

  • fa / fb own through distinct mod-qualified Impl nodes (a.Inner / b.Inner) — fails pre-fix (methods orphan to File).
  • Negative guard: scoped-generic fd is not owned through a phantom node (deferred scope-out).
  • Worker-pool parity block (usedWorkerPool === true).

Verification

  • rust resolver suite: registry-primary leg 174/174, legacy leg 172 + 2 by-design skips — both green, including the worker path.
  • tsc --noEmit clean; prettier clean.
  • rust-captures-golden regenerated additively (4 insertions — the new fixture only); scope-capture tripwire unaffected.

Closes #1992.

🤖 Generated with Claude Code

…d-qualified Impl node (#1992)

A generic inherent-impl target (`impl<T> Inner<T>`) is a `generic_type` node, which the inherent-impl owner walk (findEnclosingClassInfo) did not match — so the walk returned null and the method got `File -> DEFINES` with NO HAS_METHOD edge (orphaned, and invisible to findDanglingEdges). The Impl node was already correctly mod-qualified (the @name capture drills into the inner type_identifier, tree-sitter-queries.ts), so this is an owner-walk-only fix: drill into the generic base and mirror the node gate so the owner id == the node id byte-for-byte. A scoped-generic target (`impl<T> a::Inner<T>`) materializes no Impl node and is left orphaned (deferred) rather than minting a phantom owner.

The owner walk is shared by the sequential and worker paths. New fixture + tests assert positive HAS_METHOD ownership through distinct `a.Inner` / `b.Inner` nodes on both resolver legs and the worker path, plus a negative scoped-generic guard. rust-captures-golden regenerated additively for the new fixture.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@magyargergo

Copy link
Copy Markdown
Collaborator Author

Tri-review — approve

Method: per-PR multi-lens review → independent adversarial re-verification → static-analysis alignment (tsc/eslint/prettier, all clean) → cross-stack synthesis + critic gate.

Review: PR #2003 — Rust generic inherent-impl ownership (#1992)

Verdict: Approve

This is a tight, well-scoped, well-tested bugfix. Generic Rust inherent-impl methods (impl<T> Inner<T>) were being orphaned to File because the owner-walk in findEnclosingClassInfo only matched type_identifier/scoped_type_identifier, never generic_type. The fix drills into the generic base and mirrors the three @definition.impl materialization queries exactly.

What I verified (real runtime path)

  • Query↔helper parity is exact. tree-sitter-queries.ts:1225-1230 materializes an Impl node for precisely three base shapes — bare type_identifier, generic_type type:(type_identifier), and scoped_type_identifier (grep confirms no other @definition.impl). The three helper arms map 1:1.
  • Owner-id == node-id holds byte-for-byte. The Impl node id (parsing-processor.ts:660-665, copied verbatim into parse-worker.ts:1864-1869) is keyed via qualifyRustImplTargetByModScope(...) gated on nameNode?.type === 'type_identifier'. For the generic query the @name capture is the inner type_identifier, so the node IS mod-qualified — and the helper computes the same qualifyRustImplTargetByModScope(current, baseType.text) off generic_type.childForFieldName('type'). Crucially, classTemplateTag (parsing-processor.ts:792-801) is gated to Class/Struct/Interface/Enum/Record and excludes Impl, so an Impl node carries no template/arity tags even for impl<T> Inner<T> — the invariant survives.
  • Generic-over-scoped correctly orphans. impl<T> crate::c::Scoped<T> falls through (the scoped id is nested under generic_type, not a direct child), mod_item is not a CLASS_CONTAINER, so the walk returns null. The negative test asserts fd has no HAS_METHOD owner.
  • Sequential ≡ worker (byte-identical keying code) and both registry-primary legs (HAS_METHOD is structure-phase, pre-resolver-split; tests use plain it).
  • tsc --noEmit clean; CHANGELOG untouched; golden change is additive (one new auto-enumerated rust- fixture entry).

Strengths

  • Positive node-identity assertions (sourceId.toContain('a.Inner')/'b.Inner', distinct), not dangle-only — these genuinely fail on the pre-fix base.
  • Real worker-pool parity guard (usedWorkerPool === true) + a negative phantom-owner guard.
  • Excellent, intent-revealing comments documenting exactly why each arm exists and why the scoped-generic case is deferred.

Findings (all minor/nit — none blocking)

  • [minor] Language-naming in shared core/ingestion (ast-helpers.ts:513-545): the new arm names impl_item/generic_type directly. This is a pre-existing pattern (the function already special-cases Go and the Rust for/scoped/unscoped arms by node type) and impl_item is Rust-only, so it cannot affect other languages — but a follow-up could migrate the Rust block behind the existing resolveEnclosingOwner hook.
  • [nit] Lane diff carries an unrelated dependabot lru-cache bump (package-lock.json) because the branch base is one commit behind main; resolves on rebase. The fix commit itself touches only the 4 expected files.
  • [nit] fd orphan negative assertion is only on the sequential leg — add it to the worker describe for symmetric coverage (one line).
  • [nit] implTarget.type !== 'generic_type' guard on the scoped arm is technically redundant but correct and clarifying; no change needed.

Nice work — this is a model targeted ingestion fix with the right test shape.

@github-actions

github-actions Bot commented Jun 3, 2026

Copy link
Copy Markdown
Contributor

CI Report

All checks passed

Pipeline Status

Stage Status Details
✅ Typecheck success tsc --noEmit
✅ Tests success unit tests, 3 platforms
✅ E2E success gitnexus-web changes only

Test Results

Tests Passed Failed Skipped Duration
11123 11107 0 16 589s

✅ All 11107 tests passed

16 test(s) skipped — expand for details
  • COBOL pipeline benchmark > scales with file count
  • C++ ADL emit benchmark > emit phase scales sub-quadratically with co-scaled files and sites
  • C++ pipeline benchmark > scales with file count
  • C# pipeline benchmark > scales with file count — namespaces spread across the solution
  • C# pipeline benchmark > scales with file count — all types in one (global) namespace bucket
  • C# pipeline benchmark > scales with file count — all types in one (named) namespace bucket
  • Go pipeline benchmark > scales with file count (workers enabled)
  • Go pipeline benchmark — worker pool (issue Worker idle timeout kills long Go scope extraction and surfaces as Napi::Error during analyze #1848) > does not quarantine the large generated Go file on sub-batch idle timeout
  • Go structural interface detection benchmark > scales linearly with interface × struct count
  • Go structural interface detection split-phase benchmark > separates index-build and detection time
  • PHP pipeline benchmark > scales with file count (workers enabled)
  • Ruby pipeline benchmark > scales with file count (workers enabled)
  • Rust pipeline benchmark > scales with file count (workers enabled)
  • Vue pipeline benchmark > scales with component count
  • run.cjs direct-exec entrypoint (fix(cli): steer docs, skills, and hooks through a CLI-neutral project-local runner (#1939) #1945) > resolves a .cmd shim via the Windows shell branch, passing args and exit code
  • buildTypeEnv > known limitations (documented skip tests) > Ruby block parameter: users.each { |user| } — closure param inference, different feature

Code Coverage

Tests

Metric Coverage Covered Base Delta Status
Statements 80.37% 38879/48374 79.84% 📈 +0.5 🟢 ████████████████░░░░
Branches 68.93% 24717/35857 68.5% 📈 +0.4 🟢 █████████████░░░░░░░
Functions 85.59% 4047/4728 84.94% 📈 +0.7 🟢 █████████████████░░░
Lines 83.97% 34984/41661 83.36% 📈 +0.6 🟢 ████████████████░░░░

📋 View full run · Generated by CI

@magyargergo

Copy link
Copy Markdown
Collaborator Author

Operational note: embedding reindex after this stack lands

CachedEmbedding is keyed by nodeId (embeddings/types.ts). #1992 qualifies a generic impl's target by its mod scope, which re-points HAS_METHOD ownership and re-keys the affected method ids (bare Inner → mod-qualified). A default incremental re-analyze regenerates their embeddings, but a clean re-analyze after the stack merges is the safe call, consistent with the #1978/#1981/#1982 precedent.
(Same-tail bare-className method-id disambiguation remains a pre-existing follow-up — see the tri-review plan; not a regression introduced here.)

@vercel

vercel Bot commented Jun 4, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
gitnexus Ready Ready Preview, Comment Jun 4, 2026 7:28am

Request Review

magyargergo and others added 2 commits June 4, 2026 05:52
…regen rust bench baseline (#1992)

F3 follow-up to #1992: two same-tail generic inherent impls under sibling mods
that ALSO share a method name (`mod a { impl Inner { fn m } }` +
`mod b { impl Inner { fn m } }`) keyed the method node id `${className}.${name}`
with the bare tail (`Inner.m`) and collapsed onto one Function node (graph addNode
is first-write-wins), silently dropping the second. The owner Impl `classId` was
already mod-qualified, masking the collision behind distinct HAS_METHOD sources.
Qualify `className` (`a.Inner` / `b.Inner`) in the bare inherent-impl arm so the
node id inherits the mod scope; symmetric with the call-resolution fallback, and
the HAS_METHOD owner anchors on the unchanged qualified classId. New
same-method-name fixture + sequential & worker-parity tests; holds on both legs.

Also regenerate the rust scope-capture bench baseline: the new
rust-nested-tail-collision-generic (#1992) + rust-generic-impl-same-method-name
(F3) fixtures grow the rust-* corpus, so the order-independent fingerprint drifts
(56ffc1c0 -> b00aea0f, fixture_count 127 -> 129). Pure fixture-corpus drift — no
scope-extractor change; existing fixtures' captures byte-identical.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…me fixture (#1992)

The rust-* scope-capture corpus is fingerprinted by TWO gates: the bench baseline
(bench/scope-capture/baselines.json, already updated) and the rust-captures-golden
unit test (test/fixtures/rust-captures-golden/expected-captures.json). Adding the
F3 fixture rust-generic-impl-same-method-name grew the corpus 128->129 entries, so
the committed golden drifted too. Regenerated additively (UPDATE_GOLDEN=1) — only
the new fixture's entry is added; existing fixtures' captures are byte-identical.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@magyargergo magyargergo merged commit c11f50a into main Jun 4, 2026
31 checks passed
@magyargergo magyargergo deleted the fix/1992-rust-generic-impl-ownership branch June 4, 2026 07:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix(ingestion): qualify Rust GENERIC inherent-impl target by mod scope (same-tail impl Inner<T> collapse)

1 participant